-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Icon: Improve icon
prop usage docs in Storybook
#67280
Conversation
560092e
to
31cb1fc
Compare
code: ` | ||
<Icon | ||
icon={ ( { size } ) => ( | ||
<img |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid manual snippets as much as possible, but I think it's unavoidable in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish they were all properly formatted.
Right 😅 The automatic code snippet engine leaves a lot to be desired. I feel like we might need to start investing more in manual code snippets, since some of them are quite inscrutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can layer prettier on top of it 😉
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stories look good, but I don't see className
mentioned anywhere, and since we remove it from docs in #67282, this feels like a gap.
code: ` | ||
<Icon | ||
icon={ ( { size } ) => ( | ||
<img |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -47,26 +47,68 @@ FillColor.args = { | |||
...Default.args, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a control to add a classname anywhere in the stories. Should we have one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts on the className
issue are here. I would prefer to align with the standard docgen behavior (i.e. allowed element props are not listed out unless explicitly highlighted).
Although if we get signals that the lack of className
documentation is causing confusion, I'm also open to standardizing on "always document className
explicitly".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much documentation is rarely an issue, while too little docs has often been an issue. In favor of extenders, I'd personally suggest proactively documenting all props, even if they're general and available for every component. Not something to block this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyxla , while I agree with you in general, I'm not sure it would be a good idea for our use-case. "All props" means, in many cases, every prop that the underlying HTML element can accept:
- if we list them all, the docs would become very long and it would be hard for consumers of the component to spot the "main" props;
- if we don't list them all, how do we draw a line? It feels very arbitrary and potentially even more confusing, since a user may well assume that if a prop is not listed, it can't be specified.
Can we find an alternative solution, maybe adding a paragraph where we explain that the component also accepts props X Y Z (or "every valid attribute and property for the X HTML element")?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All props might be a bit too much, I agree. But useful props folks want to use most often make sense to be documented IMHO.
Can we find an alternative solution, maybe adding a paragraph where we explain that the component also accepts props X Y Z (or "every valid attribute and property for the X HTML element")?
That could work too 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both prettifying the snippets and documenting some more of the general props are things that can and should be tackled separately.
This one looks good, let's 🚢 , thanks! 🚀
In preparation for #67242
What?
Improve the
icon
prop usage examples in the Storybook forIcon
.(The overall type docs and readme are improved in #67282.)
Why?
The "With a Function" and "With a Component" examples didn't really make sense, and didn't demonstrate how the
size
prop is passed.Testing Instructions
See the Storybook docs for the
Icon
component in@wordpress/components
.Screenshots or screencast